Limit the element count of fixed-length lists#2537
Conversation
Fixed-length lists were only checked for zero elements, with no upper bound, so a list of up to 4294967295 elements passed validation. The count then flows to consumers and overflows their canonical ABI size calculations. Cap the element count at 1 Gi, the limit suggested on the issue. Closes bytecodealliance#2416.
|
While here I noticed this bounds the count rather than the resulting byte size. |
|
Thanks for the PR, but yeah what I was looking for in the original issue was a bound on the byte-size of the list so downstream consumers don't have to worry as much about integer overflow. In that sense I think the check here should be on the in-memory-byte-size rather than the number of elements. |
|
Switched the check to the in-memory byte size, element ABI size times length, with a 1 GiB limit. Two questions before I finish it. Computing the element size means walking the element type, so a deeply nested chain of fixed-length lists can blow the stack. Want the size stored per type next to the existing The limit also rejects |
|
The walk should be fine, yeah, we've got other mechanisms of limiting the depth of a type for this exact reason. Most of wasm-tools/external tooling will walk the structure of a type recursively so the maximum depth is bounded. If you're actually blowing the stack though and this isn't hypothetical that may mean we have a hole in our checks somewhere... Otherwise though yeah feel free to change the |
|
Confirmed it's real, not hypothetical. A nested fixed-length-list chain overflows the stack on the binary validate path around 80k deep where the element-count version was fine, and nothing bounds that depth ( |
|
Ah ok I'm mistakenly remembering where things are. Wasmtime has a depth check but wasmparser does not, and that's why I thought the issue would be solved here (I thought it was here, too). In light of that WDYT about maybe redefining Although... saying all that out loud, maybe it's just best to be relatively restrictive on fixed-length-list sizes. It's probably not actually true that being off by a small factor wouldn't matter in practice because when multiplying by the length of a list that blows up the error factor here and magnifies any miscalculation. Stepping back a bit, the general goal of the issue here is to ensure that overflow checks aren't needed anywhere else in the system that handles fixed-length-lists and it's just the validator here that might need overflow checks. Just having a fixed limit on the size of a list isn't sufficient for this because eventually the calculation for "how big is this list" is going to overflow. Without precisely tracking in-memory size, though, I'm not sure if it's possible to do that otherwise. I forget if wasmparser is otherwise handling canonical ABI layout anywhere right now (I don't think it is, that feels more a wit-parser thing). Given all that, it might be the case that preventing overflows is basically just not possible here. If that's the case then I suspect that this PR may be the best way forward of "some fixed limit that's big enough for everyone" to avoid "infinity" cases, but otherwise all downstream usage of fixed-length-lists will still need to handle overflow. The only other alternative I can think of is to precisely track canonical ABI size in a manner similar to (sorry for the sort of stream-of-consciousness) |
2 participants
Fixed-length lists were only checked for zero elements, with no upper bound, so a list of up to 4294967295 elements passed validation. The count then flows to consumers and overflows their canonical ABI size calculations. Cap the element count at 1 Gi, the limit suggested on the issue.
Closes #2416.